[SCons] Implement script to extract options from SConstruct #1136

ischoegl · 2021-10-30T18:30:05Z

Changes proposed in this pull request

Implement scons2rst.py, which extracts options from SConstruct via AST
Refactor SConstruct in order to provide access to platform/compiler dependent defaults
SConstruct options (including platform/compiler dependent defaults) are extracted and written to a ReST file

If applicable, fill in the issue number this pull request is fixing

If applicable, provide an example illustrating new features this pull request is introducing

$ python scons2rst.py 
$ python scons2rst.py --dev SConstruct

An example for generated output for the development branch is config-options.rst.txt

At the moment, platform-dependent options are described as <platform dependent>.

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

ischoegl · 2021-10-31T14:01:46Z

Done. For output generated by the script, refer to Cantera/cantera-website#160. As this PR stands, the parsed options are uploaded together with documentation into a new scons folder.

ischoegl · 2021-10-31T17:08:11Z

Noticed a couple of additional manual edits in the original config-options-dev.rst file, which are now also considered.

bryanwweber

Thanks @ischoegl Reworking the defaults class is certainly welcome and a nice cleanup. I do have a small concern that everything is equivalent for the compilers that we don't run on GH Actions (Cygwin, mingw, Intel), since some of that stuff broke when I updated buildutils.py. Is there any way we can address that?

SConstruct

bryanwweber · 2021-11-01T16:17:30Z

SConstruct

-        'Set this to the directory where Cantera should be installed.',
+        "prefix",
+        """Set this to the directory where Cantera should be installed. On Windows
+           systems, '$ProgramFiles' typically refers to "C:\Program Files".""",


Can you clarify what "typically" means here? If a user doesn't know what '$ProgramFiles' is or what it does, how should they interpret this help?

This should become clear from the context once the ReST is assembled ... the corresponding portion is

.. _prefix: * ``prefix``: [ ``path/to/prefix`` ] Set this to the directory where Cantera should be installed. On Windows systems, ``$ProgramFiles`` typically refers to ``"C:\Program Files"``. - default: platform dependent - Windows: ``'$ProgramFiles\Cantera'`` - Otherwise: ``'/usr/local'``

The original (hand-coded) ReST explicitly referred to C:\Program Files\Cantera, but that is not what SConstruct actually implements.

OK, the ReST looks good, but does scons help return all that context too?

Point taken!

SConstruct

bryanwweber · 2021-11-01T16:20:23Z

SConstruct

        '', PathVariable.PathAccept),
    EnumVariable(
        'lapack_names',
        """Set depending on whether the procedure names in the specified
           libraries are lowercase or uppercase. If you don't know, run 'nm' on
-           the library file (for example, 'nm libblas.a').""",
+           the library file (for example, "nm libblas.a").""",


This would be OK to leave as ' since it's representing a program/CLI thingy, and that's our convention elsewhere.

That's unfortunately a little tricky, as it's very hard to guess what single quotes enclose (for example, you'd have to catch exceptions for all kinds of contractions "isn't", "doesn't", "can't", "you'd", "that's"). My way around this was to limit single quote extents to whatever doesn't include whitespace. So using double quotes here is imho the easier solution (and it is still clear from a CLI perspective).

site_scons/scons2rst.py

ischoegl · 2021-11-01T16:46:19Z

@bryanwweber ... thanks for the comments!

I do have a small concern that everything is equivalent for the compilers that we don't run on GH Actions (Cygwin, mingw, Intel), since some of that stuff broke when I updated buildutils.py. Is there any way we can address that?

Not sure - what I did here is to map the current logic of SConstruct, where I explicitly didn't want introduce any changes (the refactoring was mainly done so I could parse for default options). For whatever is not on GH Actions, we appear to rely on user feedback, so it's hard to cover all of that ... we may want to state that we only test for certain compilers, although others are theoretically covered. Longterm, I hope that we can get Cantera/enhancements#117 et al. to take care of this problem.

PS: It is correct that this assumes that g++ options remain the same across toolchains, which I believe is reasonable. There is one Cygwin override but as far as I can tell the logic isn't complex enough to warrant nested option dictionaries.

bryanwweber · 2021-11-01T17:26:20Z

Not sure - what I did here is to map the current logic of SConstruct, where I explicitly didn't want introduce any changes

Yes, I know, but in any refactoring there's a chance of introducing bugs or incidentally changing behavior, I just hope that doesn't happen here. I guess the resolution is just Cantera/enhancements#117.

we may want to state that we only test for certain compilers, although others are theoretically covered.

That's a good idea, I wonder where that should go... probably in the compiling docs on the website would be a good spot.

I'm going to move the larger conversation about the home for this functionality out here into the main thread, so hopefully it doesn't get collapsed by GitHub quite so quickly... I never said, thank you @ischoegl for taking on the first implementation of this! Clearly that issue has been around for almost 3 years, so it's nice to finally have something concrete to discuss.

So, on looking through the scons2rst.py script a little more thoroughly, I think that @speth was correct in Cantera/cantera-website#26 (comment), insofar as I think it'd be better to include an option to get this output directly into SConstruct. The reason for that is SCons already knows how to parse its options and get default values, see site_scons/buildutils.py:help.

I think it would be considerably less fragile to have a new SCons option (like scons build etc.) that produces the appropriate output to stdout and this can be saved by anyone to a file. I'm worried that with a separate script and using the AST, that API changes made by SCons will require rewriting the conversion script, and possibly supporting multiple versions of SCons with such a script; or, that refactoring of SConstruct by us will cause the conversion script to fail. That just increases our already overloaded capacity for maintenance.

Like scons help, this new option could be something near the top of SConstruct that simply calls a function in buildutils and then exits. The only trick would be making sure that platform-specific options get included, regardless of what platform is running scons get-configuration. Maybe a separate all_options list that updates the environment if get-configuration is in COMMAND_LINE_TARGETS.

Once that new option is added to SConstruct here, we can add a plugin to the website repo to run scons get-options (or whatever the name is) and save that output to the config-options-dev.rst (or whatever that filename is).

ischoegl · 2021-11-01T17:50:32Z

@bryanwweber ... thanks for the additional feedback. My initial reaction is that I am not sure that I agree with that assessment. The nice thing about AST is that you know what the original expressions are that are plugged into other expressions, while leaving things in plain code.

The alternative would be to build a mechanism that detects options for scons get-options directly, meaning that the construction of parameter lists would happen in SConstruct (imho, a bad idea), or be pushed into buildutils, which hides what is done from the user.

Either way, much of the work that is already done can be leveraged - the Defaults object will remain the same (although perhaps moved to buildutils), and the object handling the ReST formatting can be copied directly over. The difference is honestly not as big as it appears ... the main price for not using AST is that more is moved out of SConstruct, which may be a good thing after all.

bryanwweber · 2021-11-01T20:25:39Z

The alternative would be to build a mechanism that detects options

If I understand your concern correctly, this is already done by SCons when opts.AddVariables is called. The Variables object stores all the options and their defaults. This is how the help() function works. The only trick will be to make sure that all the variables get added, even if they're not available on the platform where the generator is being run (for example, running scons get-options on Linux, but it has to include options for Windows).

ischoegl · 2021-11-01T20:45:59Z

this is already done by SCons when opts.AddVariables is called. The Variables object stores all the options and their defaults. This is how the help() function works.

That's not exactly what happens for platform/compiler-dependent options, as the default option is not known a priori. The way around this that I can see is to implement wrappers, that can generate SCons variables, but also output ReST. (e.g. wrap PathVariable in a PathOption that is defined in buildutils).

It's much easier to resolve for the scons help function as you already know what compiler/platform you are using 😉

ischoegl · 2021-11-01T21:25:13Z

Overall, I'm 👍 with adding scons get-options, but that will definitely mean more lines to review ...

ischoegl · 2021-11-02T22:34:46Z

Superseded by #1137

ischoegl force-pushed the extract-scons-options branch 6 times, most recently from 1fff9c0 to 15690ce Compare October 31, 2021 03:58

ischoegl added 2 commits October 30, 2021 23:03

[SCons] Update/fix SConstruct formatting

529d6da

[SCons] Implement script to extract options from SConstruct

4331ecd

ischoegl mentioned this pull request Oct 31, 2021

Update SCons configuration options to version 2.5.1 Cantera/cantera-website#160

Merged

ischoegl force-pushed the extract-scons-options branch 3 times, most recently from 58769ea to ef61487 Compare October 31, 2021 13:19

ischoegl requested a review from a team October 31, 2021 14:02

ischoegl force-pushed the extract-scons-options branch 3 times, most recently from bd1b4db to 8f4a299 Compare October 31, 2021 15:18

[SCons] Refactor default option selection in SConstruct

e164ec5

ischoegl force-pushed the extract-scons-options branch 2 times, most recently from 9e05d9f to bba50ea Compare October 31, 2021 16:43

ischoegl added 3 commits October 31, 2021 11:59

[SCons] Capture default options in scons2rst.py

7f1c2a3

[CI] Parse SConstruct options

7d57d1a

[CI] Store artifact from docs build

91e0db8

ischoegl force-pushed the extract-scons-options branch from bba50ea to 91e0db8 Compare October 31, 2021 16:59

bryanwweber requested changes Nov 1, 2021

View reviewed changes

[SCons] Switch CamelCase to snake_case for defaults

42a1cbf

ischoegl added 3 commits November 1, 2021 15:13

[SCons] Document pch_flags defaults

1d500a0

[SCons] Document openmp_flag defaults

adf0ba0

[SCons] Update comments

71ebed1

ischoegl force-pushed the extract-scons-options branch from 201b880 to 71ebed1 Compare November 1, 2021 20:13

[SCons] Require SCons >= 3.0.0

54e2600

ischoegl force-pushed the extract-scons-options branch 4 times, most recently from 0adfc6c to 2e1ffd1 Compare November 2, 2021 05:16

ischoegl mentioned this pull request Nov 2, 2021

Refactor SConstruct option handling / extract scons options v2 #1137

Merged

5 tasks

ischoegl force-pushed the extract-scons-options branch from 2e1ffd1 to 54e2600 Compare November 2, 2021 22:33

ischoegl closed this Nov 2, 2021

ischoegl deleted the extract-scons-options branch January 8, 2022 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SCons] Implement script to extract options from SConstruct #1136

[SCons] Implement script to extract options from SConstruct #1136

ischoegl commented Oct 30, 2021 •

edited

Loading

ischoegl commented Oct 31, 2021

ischoegl commented Oct 31, 2021 •

edited

Loading

bryanwweber left a comment

bryanwweber Nov 1, 2021

ischoegl Nov 1, 2021 •

edited

Loading

bryanwweber Nov 1, 2021

ischoegl Nov 1, 2021

bryanwweber Nov 1, 2021

ischoegl Nov 1, 2021 •

edited

Loading

ischoegl commented Nov 1, 2021 •

edited

Loading

bryanwweber commented Nov 1, 2021 •

edited

Loading

ischoegl commented Nov 1, 2021

bryanwweber commented Nov 1, 2021

ischoegl commented Nov 1, 2021 •

edited

Loading

ischoegl commented Nov 1, 2021

ischoegl commented Nov 2, 2021

[SCons] Implement script to extract options from SConstruct #1136

[SCons] Implement script to extract options from SConstruct #1136

Conversation

ischoegl commented Oct 30, 2021 • edited Loading

ischoegl commented Oct 31, 2021

ischoegl commented Oct 31, 2021 • edited Loading

bryanwweber left a comment

Choose a reason for hiding this comment

bryanwweber Nov 1, 2021

Choose a reason for hiding this comment

ischoegl Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

bryanwweber Nov 1, 2021

Choose a reason for hiding this comment

ischoegl Nov 1, 2021

Choose a reason for hiding this comment

bryanwweber Nov 1, 2021

Choose a reason for hiding this comment

ischoegl Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

ischoegl commented Nov 1, 2021 • edited Loading

bryanwweber commented Nov 1, 2021 • edited Loading

ischoegl commented Nov 1, 2021

bryanwweber commented Nov 1, 2021

ischoegl commented Nov 1, 2021 • edited Loading

ischoegl commented Nov 1, 2021

ischoegl commented Nov 2, 2021

ischoegl commented Oct 30, 2021 •

edited

Loading

ischoegl commented Oct 31, 2021 •

edited

Loading

ischoegl Nov 1, 2021 •

edited

Loading

ischoegl Nov 1, 2021 •

edited

Loading

ischoegl commented Nov 1, 2021 •

edited

Loading

bryanwweber commented Nov 1, 2021 •

edited

Loading

ischoegl commented Nov 1, 2021 •

edited

Loading